Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data management improvements #259

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Data management improvements #259

merged 1 commit into from
Dec 9, 2024

Conversation

hweihwang
Copy link
Contributor

No description provided.

@hweihwang hweihwang requested a review from juliusknorr November 1, 2024 10:23
@hweihwang hweihwang linked an issue Nov 1, 2024 that may be closed by this pull request
@hweihwang hweihwang force-pushed the feat/quarantine-recovery branch from fe06579 to e4fd9bc Compare November 5, 2024 11:10
@hweihwang hweihwang marked this pull request as ready for review November 5, 2024 11:11
@hweihwang hweihwang requested a review from grnd-alt as a code owner November 5, 2024 11:11
@hweihwang hweihwang force-pushed the feat/quarantine-recovery branch 5 times, most recently from 22072bb to 67cd2db Compare November 12, 2024 10:32
@hweihwang hweihwang mentioned this pull request Nov 25, 2024
Copy link
Member

@grnd-alt grnd-alt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 code quality is good, and changes would reduce reported errors, great work.

I would think having file operations for each websocket sync will increase performance loads significantly. have you thought about doing the backups only every X amount of syncs?

websocket_server/BackupManager.js Show resolved Hide resolved
@hweihwang
Copy link
Contributor Author

hweihwang commented Dec 2, 2024

@grnd-alt Really valid points on server/files operations there, I thought about that too, the current implementation had some tweaks for that:

  • We had a file locking mechanism with a default timeout of 5 seconds (this.lockTimeout = options.lockTimeout || 5000) to prevent too frequent writes
  • The backup operations are called asynchronously and not really block the main thread:
if (updatedData) {
    await this.updateRoomWithData(room, updatedData, users, lastEditedUser)
    this.createRoomBackup(room.id, room)
}

=> Would be better if can move it to a separate process (a queue processor) to process saving since it's not really critical

  • I currently think of incorporating with some client side storage (IndexedDB/LocalStorage) to reduce some server loads/operations and prevent data loss too, since one user might only access few rooms at a time compared to server with lots amount of data there. Let me know your thoughts on this one @grnd-alt , thank you!

@hweihwang hweihwang force-pushed the feat/quarantine-recovery branch from 67cd2db to 16ae3a5 Compare December 2, 2024 07:29
@hweihwang hweihwang requested a review from grnd-alt December 2, 2024 07:39
@grnd-alt
Copy link
Member

grnd-alt commented Dec 2, 2024

@hweihwang Yes storing data in localstorage is a good idea, I think we should investigate it later on, as it might get complex to have divergent versions from multiple users, that need to be reconciled when restoring from the browser storage.

Copy link
Member

@grnd-alt grnd-alt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 as said before the code is great.
IndexDB to be discussed later

@juliusknorr
Copy link
Member

Anything still pending for the merge?

@hweihwang
Copy link
Contributor Author

hweihwang commented Dec 6, 2024

I'm still thinking a bit about the approach since the board can still work isolatedly without much needed from nextcloud server
=> So user might no longer needed to correct their configurations to connect from the whiteboard server to nextcloud server
=> Data will be stay in the node server and never go to nextcloud storage if those connection issues not resolveed
=> Might introduce some security issues about read and write permissions to the file
=> ...

Not sure they're 100% related or might be in another ticket (Since users still need to aqquire jwt token from nextcloud server frequently but that's from FE => NC directly). Will think a bit more about that and will adjust the implementation at least to resolve some of. Want to know your thoughts on this @juliusknorr @grnd-alt too! Thank you!

@hweihwang hweihwang merged commit d84577b into main Dec 9, 2024
26 checks passed
@hweihwang hweihwang deleted the feat/quarantine-recovery branch December 9, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done
Development

Successfully merging this pull request may close these issues.

Quarantine files that could not be saved to a local directory
3 participants